-
Notifications
You must be signed in to change notification settings - Fork 1.4k
YAML config: Supports parsing array format #5126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Thanks for this improvement! |
Gett error when config array of basic data types with fluentd.yaml, eg:
or This feature is to address this issue. |
I see. There seems to be a bug. I have confirmed the following behaviors.
A: Can parse Array format for
|
Hmm, it looks like the current implementation intentionally treats only fluentd/lib/fluent/config/yaml_parser/parser.rb Lines 145 to 163 in 3c85506
|
It’s still unclear whether the current implementation is intentional or an oversight (possibly a bug).
|
We supported YAML in the following: There hasn’t been any particular discussion about handling Arrays. There are no test cases for it, but it’s clearly documented. |
The following code is to parse the multiple sections. fluentd/lib/fluent/config/yaml_parser/parser.rb Lines 146 to 149 in 3c85506
|
There are two cases for Arrays: one where they should be recognized as sections, and another where they should be treated as single options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
The direction of this fix looks good!
Since the current implementation assumes either an Array or a Hash as a section, how about checking it like this?
I’d appreciate it if you could add some test cases. Line 161 in dc21d83
|
And could you add DCO? https://github.com/fluent/fluentd/pull/5126/checks?check_run_id=53146586771 |
Whether to backport this needs to be discussed. |
efe72e6
to
c160e7d
Compare
…n files. Signed-off-by: Hear_Y <[email protected]>
Co-authored-by: Daijiro Fukuda <[email protected]> Signed-off-by: Hear_Y <[email protected]>
Co-authored-by: Daijiro Fukuda <[email protected]> Signed-off-by: Hear_Y <[email protected]> Supports parsing array of basic data types when analyzing YAML configuration files. Signed-off-by: Hear_Y <[email protected]>
c160e7d
to
2264e5e
Compare
Thanks! Test code is difficult for me. I have add DCO. I have no other question. Please help to close this Commit if necessary. |
Thanks! |
Signed-off-by: Daijiro Fukuda <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM.
Which issue(s) this PR fixes:
None.
What this PR does / why we need it:
Supports parsing array of basic data types when analyzing YAML configuration files.
The current behavior of Fluentd:
$args
only.retryable_response_codes: 503, 504
retryable_response_codes: "503"
retryable_response_codes: 503,
This supports YAML Array format for all options.
Note: This PR doesn't address the issue where setting an Int directly to an Array option causes ConfigError:
retryable_response_codes: 503
(TheC
case in #5126 (comment))It could be a different issue.
Docs Changes: Not Need
Release Note: Same as the title.